pkg/cli/admin/mustgather/mustgather.go (1)

834-854: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add/update unit test coverage for the new timeout scoping behavior.

This change alters lifecycle timing and error-suppression logic but no corresponding test changes are included. processNextWorkItem is exercised via MustGatherOptions, so a table-driven test simulating a slow/hanging log stream (context deadline expiring mid-stream) would validate that the command now returns promptly with "gather never finished" instead of hanging, and that DeadlineExceeded from log streaming doesn't spuriously log "gather logs unavailable."
As per coding guidelines, "All changes must include unit test additions/changes."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/cli/admin/mustgather/mustgather.go` around lines 834 - 854, Add or update
unit tests for the new timeout scoping in MustGatherOptions.processNextWorkItem.
Cover a slow or hanging getGatherContainerLogs path where gatherCtx expires
mid-stream, and assert the command returns promptly with the expected "gather
never finished" behavior instead of hanging. Also verify that
context.DeadlineExceeded or context.Canceled from getGatherContainerLogs does
not trigger the "gather logs unavailable" log path, while non-context errors
still do.

Source: Coding guidelines

pkg/cli/admin/mustgather/mustgather.go (1)

953-991: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Per-phase timeout budgets can compound beyond the documented --timeout semantics.

getGatherContainerLogs now derives its own gatherCtx with a fresh o.Timeout budget, on top of waitForGatherContainerRunning (line 1058, unchanged) already independently applying its own o.Timeout via wait.PollUntilContextTimeout. If waitForGatherToComplete follows the same pattern, the overall "gather" phase in processNextWorkItem (container-start wait → log stream → completion wait) could take up to ~3× o.Timeout in the worst case, not the single bounded o.Timeout that the flag's own help text promises ("This timeout only applies to the data gathering phase"). This is the same concern previously raised by reviewers (tchap: "we don't need to use PollUntilContextTimeout and o.Timeout within this function"; afcollins: "I think the user experience of setting --timeout is that I expect the command to terminate after that timeout is reached"), and it does not appear resolved here.

Separately, the comment at lines 954-956 claims gatherCtx covers "the entire gather lifecycle (waiting for container start, streaming logs, waiting for completion)", but gatherCtx is scoped only to this function's log-streaming/isGatherDone loop — waitForGatherContainerRunning and waitForGatherToComplete (called before/after, in processNextWorkItem) still use the plain ctx with their own separate o.Timeout windows. The comment should be corrected to reflect the actual (per-substep) scope, or the design should be changed to derive one shared deadline-scoped context in processNextWorkItem and pass it through all three phases (dropping the redundant internal o.Timeout usage in each), consistent with the earlier reviewer feedback.

Suggested direction (illustrative, needs coordination across the three phases)

 func (o *MustGatherOptions) processNextWorkItem(ctx context.Context, ns string, pod *corev1.Pod) error {
+	// Single deadline covering the whole gather phase (start-wait, log
+	// streaming, completion-wait). Copying intentionally continues on ctx.
+	gatherCtx, gatherCancel := context.WithTimeout(ctx, o.Timeout)
+	defer gatherCancel()
 	...
-	if err := o.waitForGatherContainerRunning(ctx, pod); err != nil {
+	if err := o.waitForGatherContainerRunning(gatherCtx, pod); err != nil {
 	...
-	if err := o.getGatherContainerLogs(ctx, pod); err != nil {
+	if err := o.getGatherContainerLogs(gatherCtx, pod); err != nil {
 	...
-	if err := o.waitForGatherToComplete(ctx, pod); err != nil {
+	if err := o.waitForGatherToComplete(gatherCtx, pod); err != nil {

And drop the now-redundant internal context.WithTimeout(ctx, o.Timeout) / o.Timeout-based polling inside getGatherContainerLogs / waitForGatherContainerRunning / waitForGatherToComplete.

As per coding guidelines, pkg/cli/**/*.go should use context.Context correctly for cancellation and timeouts — the concern here is that the current per-function contexts don't yield the single-overall-deadline behavior the --timeout flag documents.

#!/bin/bash
# Confirm whether waitForGatherToComplete independently re-applies o.Timeout
ast-grep run --pattern 'func (o *MustGatherOptions) waitForGatherToComplete($_, $_) $_ {
  $$$
}' --lang go pkg/cli/admin/mustgather/mustgather.go

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@pkg/cli/admin/mustgather/mustgather.go` around lines 953 - 991, The
gather-phase timeout handling is still using separate per-step deadlines instead
of one shared `--timeout` budget, so `processNextWorkItem`,
`waitForGatherContainerRunning`, `getGatherContainerLogs`, and
`waitForGatherToComplete` can compound their waits. Refactor the flow to derive
a single deadline-scoped context once in `processNextWorkItem` and pass it
through all three phases, removing the inner `context.WithTimeout(ctx,
o.Timeout)` / `wait.PollUntilContextTimeout` usage. Also update the
`getGatherContainerLogs` comment to match the actual scope if you keep any
substep-specific context behavior.

Source: Path instructions